Skip to content

refactor(tests): migrate from mocha to jest #2564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Jan 26, 2021
Merged

refactor(tests): migrate from mocha to jest #2564

merged 14 commits into from
Jan 26, 2021

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Jan 11, 2021 •
edited
Loading

This migrates the testing solution from mocha to jest. We were using some deprecated mocha methods in our tests so those are fixed now.

#2621 was merged into this and includes code-coverage

Screenshot

B36025DD-780B-43BA-83A2-A402CAC8212A

TODOS

  • try out @nhooyr suggestion "...a better option might be to ignore the .d.ts file of mocha from being type checked at all. iirc there's some exclude fields in tsconfig right" to see if that's a viable solution
  • fix CI issues
  • sign old commits and clean up

Related:

@jsjoeio jsjoeio self-assigned this Jan 11, 2021
@jsjoeio jsjoeio requested review from code-asher and nhooyr January 11, 2021 18:55
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021

Hmm 🤔 Looks like TS is unhappy with duplicate identifiers. I think it’s because mocha is in the subtree and jest is in our repo and both use the same identifiers in their type definitions. I’ll see what workarounds there may be.

D9A3100C-C869-4850-9960-C0FB3FEC8A13

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021 •
edited
Loading

Okay from quick research seems like there are some things we can try:

  • adding skipLibCheck to the tsconfig (Probably in lib/vscode)
  • using jest-without-globals in our repo (link)

Link to google search

@nhooyr
Copy link
Contributor

nhooyr commented Jan 11, 2021

This PR is on the wrong base @jsjoeio

@jsjoeio jsjoeio changed the base branch from v3.8.0 to master January 11, 2021 19:53
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 11, 2021 •
edited
Loading

This PR is on the wrong base @jsjoeio

Oops, fixed.

@jsjoeio jsjoeio marked this pull request as draft January 12, 2021 18:13
@jsjoeio jsjoeio marked this pull request as ready for review January 12, 2021 19:02
@jsjoeio jsjoeio force-pushed the issue-2550-migrate-mocha-jest branch from e104339 to 66f4029 Compare January 12, 2021 19:06
@nhooyr nhooyr added this to the v3.8.1 milestone Jan 18, 2021
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 19, 2021

Once/if CI passes, I will sign older commits and clean them up.

@jsjoeio jsjoeio marked this pull request as draft January 19, 2021 21:48
@jsjoeio jsjoeio force-pushed the issue-2550-migrate-mocha-jest branch 2 times, most recently from aa0cf17 to a1e4e9d Compare January 19, 2021 22:46
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 20, 2021 •
edited
Loading

Seeing some weird errors in release.

image

Possible solution: stackoverflow

Hmm...this error seems to occur a lot and relates to mismatched versions (i.e. @types/jest uses a TS feature that isn't in your TS version). I'm going to try to upgrade TS to 4.1.

UPDATE: even after upgrading TypeScript to the latest version, I'm still seeing the same errors.

UPDATE: so this error is happening because of jest-diff and pretty-format. But the issue is they're in the root node_modules folder and for some reason the vscode build step is including them. Time to figure out how to exclude them.

@nhooyr
Copy link
Contributor

nhooyr commented Jan 20, 2021

That's bizarre, why is the release process trying to build jest? cc @code-asher

@jsjoeio
feat: setup jest
cef7d42
@jsjoeio
refactor: tests from mocha to jest
de7d039
@jsjoeio
refactor: remove mocha
bea8bb0
@jsjoeio
fix: prevent mocha/jest types conlict
0a07d67 
Modify the tsconfig.json in lib/vscode/src/build.

This adds the flag skipLibCheck: true to tell TypeScript
to not type-check the declaration files at build time.

We need to add this because otherwise it checks the declaration
files and reports an error of duplicate type definitions
because we use Jest for our tests and they use Mocha and they
both use the global namespace "test" in their .d.ts files.
@jsjoeio
refactor: upgrade TS to 4.1.3
7571774
@jsjoeio
fix(TS error): add void to promise in util
f13ba94
@jsjoeio
fix(app.ts): resolve with server
6bf51ca
@jsjoeio
fix: add void for resolve in socket test
850c7e1
@jsjoeio
refactor: correct type signature in app.ts
646ee3a
@jsjoeio jsjoeio force-pushed the issue-2550-migrate-mocha-jest branch 2 times, most recently from fedbe58 to 1f0d80d Compare January 21, 2021 18:11
@jsjoeio jsjoeio marked this pull request as ready for review January 21, 2021 18:11
@jsjoeio jsjoeio requested a review from nhooyr January 21, 2021 18:11
@jsjoeio jsjoeio force-pushed the issue-2550-migrate-mocha-jest branch from d52ed61 to 883dd13 Compare January 21, 2021 21:07
@jsjoeio jsjoeio marked this pull request as draft January 22, 2021 17:48
@jsjoeio jsjoeio marked this pull request as ready for review January 25, 2021 20:07
code-asher
code-asher approved these changes Jan 25, 2021
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jsjoeio jsjoeio force-pushed the issue-2550-migrate-mocha-jest branch from bdb8b0e to 3044224 Compare January 25, 2021 23:21
@jsjoeio jsjoeio removed the request for review from nhooyr January 25, 2021 23:35
@jsjoeio jsjoeio merged commit fa548e9 into master Jan 26, 2021
@jsjoeio jsjoeio deleted the issue-2550-migrate-mocha-jest branch January 26, 2021 00:12
@jsjoeio jsjoeio mentioned this pull request Jan 26, 2021
Closed
4 tasks
@jsjoeio jsjoeio mentioned this pull request Feb 10, 2021
Closed
@jsjoeio jsjoeio added the testing Anything related to testing label May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nhooyr nhooyr nhooyr left review comments

@code-asher code-asher code-asher approved these changes

Assignees

@jsjoeio jsjoeio

Labels
testing Anything related to testing
Projects
None yet
Milestone
v3.8.1
Development

Successfully merging this pull request may close these issues.
3 participants
@jsjoeio @nhooyr @code-asher